Conversation
There was a problem hiding this comment.
Pull request overview
This pull request enables CoreCLR runtime testing in the WASM Build Tests suite by categorizing tests and filtering them appropriately based on the runtime flavor. The PR removes a CoreCLR-specific test asset that is no longer needed and ensures that tests requiring native builds, Mono-specific features, or workload installations are properly excluded when running with CoreCLR.
Changes:
- Removed the WasmBrowserRunMainOnly test asset and test class, which was specifically created for CoreCLR but is no longer needed
- Added TestCategory attributes to categorize tests as "native" (requiring AOT/native builds), "mono" (Mono-specific features), or "workload" (requiring workload installation)
- Updated test filtering logic in Wasm.Build.Tests.csproj to exclude native/mono/workload tests when running with CoreCLR runtime
- Modified WasmSdkBasedProjectProvider to expect diagnostics files for CoreCLR builds
Reviewed changes
Copilot reviewed 46 out of 46 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/mono/wasm/testassets/WasmBrowserRunMainOnly/* | Deleted entire test asset directory (6 files) that was specific to CoreCLR testing |
| src/mono/wasm/Wasm.Build.Tests/WasmBrowserRunMainOnly.cs | Deleted CoreCLR-specific test class |
| src/mono/wasm/Wasm.Build.Tests/BrowserStructures/TestAsset.cs | Removed WasmBrowserRunMainOnly test asset reference |
| src/mono/wasm/Wasm.Build.Tests/Wasm.Build.Tests.csproj | Updated XUnit trait filtering to exclude native/mono/workload categories for CoreCLR |
| src/mono/wasm/Wasm.Build.Tests/WasmSdkBasedProjectProvider.cs | Added logic to expect diagnostics.js files for CoreCLR builds |
| src/mono/wasm/Wasm.Build.Tests/WorkloadTests.cs | Added "native" category to class-level TestCategory |
| src/mono/wasm/Wasm.Build.Tests/WasmBuildAppTest.cs | Added "native" category to AOT test methods |
| src/mono/wasm/Wasm.Build.Tests/WasmSIMDTests.cs | Added "native" category to PublishSIMD_AOT test |
| src/mono/wasm/Wasm.Build.Tests/WasmNativeDefaultsTests.cs | Added "native" category to class-level TestCategory |
| src/mono/wasm/Wasm.Build.Tests/Templates/WasmTemplateTests.cs | Added "workload" and "native" categories to various template tests |
| src/mono/wasm/Wasm.Build.Tests/Templates/NativeBuildTests.cs | Added "native" category to class-level TestCategory |
| src/mono/wasm/Wasm.Build.Tests/SatelliteAssembliesTests.cs | Added "native" category to resource assembly tests |
| src/mono/wasm/Wasm.Build.Tests/PInvokeTableGeneratorTests.cs | Added "native" category to class-level TestCategory |
| src/mono/wasm/Wasm.Build.Tests/NativeRebuildTests/* | Added "native" category to all native rebuild test classes (5 files) |
| src/mono/wasm/Wasm.Build.Tests/NativeLibraryTests.cs | Added "native" category to class-level TestCategory |
| src/mono/wasm/Wasm.Build.Tests/NativeBuildTests.cs | Added "native" category to class-level TestCategory |
| src/mono/wasm/Wasm.Build.Tests/ModuleConfigTests.cs | Split SymbolMapFileEmitted test into two separate tests, marking only the emitting case as "native" |
| src/mono/wasm/Wasm.Build.Tests/MemoryTests.cs | Added "native" category to class-level TestCategory |
| src/mono/wasm/Wasm.Build.Tests/MainWithArgsTests.cs | Added "native" category to main-with-args tests |
| src/mono/wasm/Wasm.Build.Tests/InvariantTimezoneTests.cs | Added "native" category to invariant timezone tests |
| src/mono/wasm/Wasm.Build.Tests/InvariantGlobalizationTests.cs | Added "native" category to invariant globalization tests |
| src/mono/wasm/Wasm.Build.Tests/InterpPgoTests.cs | Added "mono" category to class-level TestCategory |
| src/mono/wasm/Wasm.Build.Tests/IcuTests.cs | Added "native" and "workload" categories to ICU tests |
| src/mono/wasm/Wasm.Build.Tests/IcuShardingTests*.cs | Added "native" category to ICU sharding tests (2 files) |
| src/mono/wasm/Wasm.Build.Tests/DllImportTests.cs | Added "native" category to class-level TestCategory |
| src/mono/wasm/Wasm.Build.Tests/DiagnosticsTests.cs | Added "mono" category to class-level TestCategory |
| src/mono/wasm/Wasm.Build.Tests/BuildPublishTests.cs | Added "native" category to AOT publish tests |
| src/mono/wasm/Wasm.Build.Tests/Blazor/* | Added "native" and "mono" categories to various Blazor tests (8 files) |
|
we can update this branch and try to add back the lazy loading, I just merged it |
| <HelixWorkItem Include="@(BuildWasmApps_PerJobList->'$(WorkItemPrefix)%(Identity)')" Condition="'$(RuntimeFlavor)' == 'CoreCLR' and '$(WasmFingerprintAssets)' == 'true' and '$(WasmBundlerFriendlyBootConfig)' == 'false'"> | ||
| <PayloadArchive>$(_BuildWasmAppsPayloadArchive)</PayloadArchive> | ||
| <PreCommands>$(EnvVarCommand) "HELIX_XUNIT_ARGS=-class %(Identity)"</PreCommands> | ||
| <Command>$(HelixCommand)</Command> |
There was a problem hiding this comment.
The new per-class CoreCLR work item condition relies on $(WasmFingerprintAssets) == 'true', but in CoreCLR BuildWasmApps this property may be empty (not explicitly set) even though fingerprinting defaults to enabled in the SDK/RunScriptTemplate. That would prevent these per-class items from being created, making BuildWasmAppsJobsListCLR.txt unused. Also, if WasmFingerprintAssets is set to true for CoreCLR in the future, ensure the existing fallback Wasm.Build.Tests work item (below) is suppressed to avoid running both the per-class items and the full suite.
|
Summary:
|
…romMyException", without "error".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/libraries/sendtohelix-wasm.targets:70
- For
RuntimeFlavor=CoreCLRthe per-classHelixWorkItemis created even whenTestUsingWorkloads=false, but the aggregateWasm.Build.Testswork item is also created because its condition includes$(TestUsingWorkloads) != 'true'. This will schedule the same tests twice in CoreCLR BuildWasmApps jobs. Update the conditions so CoreCLR runs either the per-class work items or the aggregate work item, but not both (e.g., gate the CoreCLR per-class items onTestUsingWorkloads=='true', or excludeRuntimeFlavor=CoreCLRfrom the aggregate condition when per-class splitting is enabled).
<HelixWorkItem Include="@(BuildWasmApps_PerJobList->'$(WorkItemPrefix)%(Identity)')" Condition="'$(RuntimeFlavor)' == 'CoreCLR' and '$(WasmFingerprintAssets)' == 'true' and '$(WasmBundlerFriendlyBootConfig)' == 'false'">
<PayloadArchive>$(_BuildWasmAppsPayloadArchive)</PayloadArchive>
<PreCommands>$(EnvVarCommand) "HELIX_XUNIT_ARGS=-class %(Identity)"</PreCommands>
<Command>$(HelixCommand)</Command>
<Timeout>$(_workItemTimeout)</Timeout>
</HelixWorkItem>
<HelixWorkItem Include="$(WorkItemPrefix)Wasm.Build.Tests" Condition="'$(TestUsingWorkloads)' != 'true' or '$(WasmFingerprintAssets)' != 'true' or '$(WasmBundlerFriendlyBootConfig)' != 'false'">
<PayloadArchive>$(_BuildWasmAppsPayloadArchive)</PayloadArchive>
<Command>$(HelixCommand)</Command>
<Timeout>$(_workItemTimeout)</Timeout>
</HelixWorkItem>
| case "DotnetRun": | ||
| case "MainWithArgs": | ||
| dotnet.run(); | ||
| dotnet.runMainAndExit(); | ||
| break; |
There was a problem hiding this comment.
dotnet.runMainAndExit() is invoked without await. Other call sites in this repo await runMainAndExit (it returns a Promise), and not awaiting here can let the script fall through without proper error/exit propagation for these scenarios. Please await the call (and ensure the switch case exits afterward if needed).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 56 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/libraries/sendtohelix-wasm.targets:64
- The CoreCLR work item condition on line 59 does not check for
TestUsingWorkloadslike the Mono condition on line 52 does. This means CoreCLR tests will run even whenTestUsingWorkloadsis 'false', while Mono tests require workloads. Is this intentional? If CoreCLR WASM requires workloads to function, the condition should include the same'$(TestUsingWorkloads)' == 'true'check. Otherwise, the test could fail in non-workload scenarios for CoreCLR.
<HelixWorkItem Include="@(BuildWasmApps_PerJobList->'$(WorkItemPrefix)%(Identity)')" Condition="'$(RuntimeFlavor)' == 'CoreCLR' and '$(WasmFingerprintAssets)' == 'true' and '$(WasmBundlerFriendlyBootConfig)' == 'false'">
<PayloadArchive>$(_BuildWasmAppsPayloadArchive)</PayloadArchive>
<PreCommands>$(EnvVarCommand) "HELIX_XUNIT_ARGS=-class %(Identity)"</PreCommands>
<Command>$(HelixCommand)</Command>
<Timeout>$(_workItemTimeout)</Timeout>
</HelixWorkItem>
| <PropertyGroup Condition="'$(Scenario)' == 'BuildWasmApps'"> | ||
| <BuildWasmAppsJobsList>$(RepositoryEngineeringDir)testing\scenarios\BuildWasmAppsJobsList.txt</BuildWasmAppsJobsList> | ||
| <BuildWasmAppsJobsList Condition="'$(RuntimeFlavor)' == 'Mono'">$(RepositoryEngineeringDir)testing\scenarios\BuildWasmAppsJobsList.txt</BuildWasmAppsJobsList> | ||
| <BuildWasmAppsJobsList Condition="'$(RuntimeFlavor)' == 'CoreCLR'">$(RepositoryEngineeringDir)testing\scenarios\BuildWasmAppsJobsListCLR.txt</BuildWasmAppsJobsList> |
There was a problem hiding this comment.
BuildWasmAppsJobsList for CoreCLR points to BuildWasmAppsJobsListCLR.txt, but the repo contains eng/testing/scenarios/BuildWasmAppsJobsListCoreCLR.txt (and no ...CLR.txt). With the current path, ReadLinesFromFile in sendtohelix-wasm.targets will never run and CoreCLR BuildWasmApps work items won’t get split per test class. Update the filename here to match the added jobs list file.
| <BuildWasmAppsJobsList Condition="'$(RuntimeFlavor)' == 'CoreCLR'">$(RepositoryEngineeringDir)testing\scenarios\BuildWasmAppsJobsListCLR.txt</BuildWasmAppsJobsList> | |
| <BuildWasmAppsJobsList Condition="'$(RuntimeFlavor)' == 'CoreCLR'">$(RepositoryEngineeringDir)testing\scenarios\BuildWasmAppsJobsListCoreCLR.txt</BuildWasmAppsJobsList> |
WasmBrowserRunMainOnly[TestCategory("native")]as necessarydotnet.diagnostics.json CoreCLRnativetrait for CoreCLR runPassing: 59 Failing: 27